Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement type merging in deconstruction with tuple literal #12526

Merged
merged 5 commits into from
Jul 20, 2016

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Jul 14, 2016

This fix reflects the LDM decision from yesterday to support deconstructions such as (string x1, byte x2, var x3) = (null, 1, 2); by hallucinating a tuple type which merges types from both sides and applying it as a conversion on the literal (which means the 1 is seen as a byte and the deconstruction succeeds).

The change is to update the method which figures the target type. It used to only consider the LHS, but now it merges types from both sides. This literal conversion is applied both in declarations and assignments.
There is also a minor tweak to improve the syntax associated with DeconstructionVariable, which is visible in the new diagnostics.

@dotnet/roslyn-compiler for review.
Relates to deconstruction work items: #11299
Relates to issue #12410

}
}
";
var comp = CompileAndVerify(source, expectedOutput: " 2 3", additionalRefs: new[] { ValueTupleRef, SystemRuntimeFacadeRef });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add a semantic info test for this scenario. We should see that the type of the (null, 2, 3) is null, converted type is (string, byte, int) and conversion is ImplicitTupleLiteral

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll add that. Thanks

@VSadov
Copy link
Member

VSadov commented Jul 14, 2016

LGTM

@gafter
Copy link
Member

gafter commented Jul 15, 2016

The change is to update the method which figures the target type. It used to only consider the LHS, but now it merges types from both sides. This literal conversion is applied both in declarations and assignments.

Why would type merging ever be needed in a Tuple assignment expression? I understood the new rule to only affect the declaration scenario.

@jcouv
Copy link
Member Author

jcouv commented Jul 15, 2016

@gafter You are correct. In an assignment, all the expressions on the left necessarily have a type, and so there is not type inference (only target typing). So in the "merging" all the types from from the left, whereas in a declaration, types can merge from both sides.
This one method handles both cases, so I didn't want to duplicate.
Would it be ok if I add some clarifying comments?

@gafter
Copy link
Member

gafter commented Jul 15, 2016

Sure

var nestedLiteral = literal.Arguments[1];
Assert.Equal(@"(1, 2)", nestedLiteral.ToString());
Assert.Null(model.GetTypeInfo(nestedLiteral).Type);
Assert.Null(model.GetTypeInfo(nestedLiteral).ConvertedType);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VSadov Could you comment on this? Is this what we should expect (null Type and ConvertedType)?
I couldn't find a test on tuple literal conversions with nested literals.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VSadov Ok to leave as-is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think (1,2) must have a type and a converted type. You should be able to extract it into a temp/method and without a type it would be impossible.
I am actually surprised this does not "just work". It is a trivial tuple literal, it should have at least the natural type...
The problem could be somewhere else.

It would be acceptable to merge this as is and enter an issue to follow up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed issue #12623

@jcouv
Copy link
Member Author

jcouv commented Jul 18, 2016

@gafter Could you do the second code review?

@jcouv
Copy link
Member Author

jcouv commented Jul 19, 2016

@dotnet/roslyn-compiler Please do a second code review. This PR is from last Thursday.

@gafter
Copy link
Member

gafter commented Jul 19, 2016

I am in an ECMA meeting today and tomorrow.

/// </summary>
private static TypeSymbol MakeTupleTypeFromDeconstructionLHS(ArrayBuilder<DeconstructionVariable> topLevelCheckedVariables, DiagnosticBag diagnostics, CSharpCompilation compilation)
private static TypeSymbol MakeMergedTupleType(ArrayBuilder<DeconstructionVariable> lhsVariables, BoundTupleLiteral rhsLiteral, CSharpSyntaxNode syntax, DiagnosticBag diagnostics, CSharpCompilation compilation)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor point: diagnostics is typically the last non-ref/out parameter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@cston
Copy link
Member

cston commented Jul 19, 2016

LGTM

{
// typeless-variable on the left and typeless-element on the right
Error(diagnostics, ErrorCode.ERR_DeconstructCouldNotInferMergedType, syntax, variable.Syntax, element.Syntax);
}
}
}
else
{
if ((object)element.Type == null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mergedType

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Fixed

@agocke
Copy link
Member

agocke commented Jul 19, 2016

LGTM

@jcouv
Copy link
Member Author

jcouv commented Jul 19, 2016

Thanks! I'll go ahead with merge.

@jcouv jcouv merged commit 5c8da03 into dotnet:master Jul 20, 2016
// For declarations, that means merging type information from the LHS and RHS
// For assignments, only the LHS side matters since it is necessarily typed
TypeSymbol lhsAsTuple = MakeMergedTupleType(checkedVariables, (BoundTupleLiteral)boundRHS, node, Compilation, diagnostics);
if (lhsAsTuple != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(object)lhsAsTuple != null

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'm still not quite sure when that is useful. Is it for certain types only (such as Symbols)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll queue that and your other suggestion in next PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for types that define operator==.

jcouv added a commit to jcouv/roslyn that referenced this pull request Jul 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants